-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Home page] [Release 4] Adding time sensitive actions for broken connections #80974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment has been minimized.
This comment has been minimized.
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
|
||
| function FixAccountingConnection({connectionName, policyID}: FixAccountingConnectionProps) { | ||
| const {translate} = useLocalize(); | ||
| const icons = useMemoizedLazyExpensifyIcons(['Exclamation'] as const); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it should work without as const
|
|
||
| function FixCompanyCardConnection({card, policyID}: FixCompanyCardConnectionProps) { | ||
| const {translate} = useLocalize(); | ||
| const icons = useMemoizedLazyExpensifyIcons(['Connect'] as const); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work without as const
This comment has been minimized.
This comment has been minimized.
| if (!shouldShowDiscount || !discountInfo) { | ||
| return null; | ||
| // Determine which offer to show based on discount type (they are mutually exclusive) | ||
| const shouldShow50off = shouldShowDiscount && discountInfo?.discountType === 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to wait until we merge this PR
And then use useTimeSensitiveOffers here
This comment has been minimized.
This comment has been minimized.
src/pages/home/TimeSensitiveSection/items/FixAccountingConnection.tsx
Outdated
Show resolved
Hide resolved
|
@ZhenjaHorbach can you share the policy data for that policy and connection? I am using standardized approach from what I know so I agree that is weird |
|
Looks like the problem with
Which return true for this policy As result, we ignore errors in this policy here Actually, do we really need |
|
Do you still get that after the latest push? |
|
And should we improve navigation on the mobile apps to navigate to the previous screen after opening 2026-01-30.22.30.56.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-01-30.22.29.26.movAndroid: mWeb Chrome2026-01-30.22.31.09.moviOS: HybridApp2026-01-30.22.29.26.moviOS: mWeb Safari2026-01-30.22.31.09.movMacOS: Chrome / Safari2026-01-30.22.21.03.mov |
|
But overall changes look good! |
Good feedback, I think that is out of scope here, but we could consider @JmillsExpensify @Expensify/design |
|
Yeah I kinda like that idea. Maybe instead of |
I think that is expected navigation using the UP button using the browser history back will take you back as you mentioned |
|
@chuckdries Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Definitely agree with that! |
|
Although cc @JmillsExpensify because I thought we decided to ditch the second line and just use one liners for all of these? |
|
I do like adding the workspace name there for clarity when admin has multiple workspaces. We can add it in follow up cc @adamgrzybowski @WojtekBoman for visibility |
|
Issue created @mountiny @shawnborton @dannymcclain |
| subtitle={translate('homePage.timeSensitiveSection.activateCard.subtitle')} | ||
| ctaText={translate('homePage.timeSensitiveSection.activateCard.cta')} | ||
| onCtaPress={() => Navigation.navigate(ROUTES.SETTINGS_WALLET_CARD_ACTIVATE.getRoute(String(card.cardID)))} | ||
| buttonProps={{success: true}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny we need to add this button prop styling to to ForYouSection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
|
This has multiple other internal reviewers and a completed reviewer checklist from @ZhenjaHorbach, so I'm un-requesting my own review |




Explanation of Change
This PR implements Release 4 of the NewDot Home page Time Sensitive section, adding two new widgets:
Key changes:
Priority order (from design doc):
Fixed Issues
$ #79970
PROPOSAL:
Tests
Prerequisite: Have a workspace with accounting integration set up
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari